[4/N] [List API] feat(gateway): expose List API from request summaries#304
[4/N] [List API] feat(gateway): expose List API from request summaries#304albertywu wants to merge 1 commit into
Conversation
60d9640 to
552ce82
Compare
61b04fe to
d0e9c2a
Compare
552ce82 to
419a487
Compare
d0e9c2a to
96cfc64
Compare
419a487 to
4980cb0
Compare
4980cb0 to
788ef8d
Compare
96cfc64 to
d308ee7
Compare
|
Few thoughts on this. 1. Group filters vs. pagination
On whether filters should be a generic 2. A possible restructureA starting point: message ListRequest {
// Required: the queue to scan (storage partition key).
string queue = 1;
// Optional. Absent = whole queue.
// Present fields AND together; a repeated field ORs within itself.
ListFilter filter = 2;
// Optional ordering; unspecified = admission order (FIFO).
ListSort sort = 3;
// Optional pagination, independent of filtering.
Page page = 4;
}
message ListFilter {
TimeWindow window = 1; // either bound 0 = unbounded
repeated string statuses = 2; // OR-set; empty = all
}
// Half-open [start, end) in Unix epoch ms. 0 = unbounded.
message TimeWindow {
int64 start_time_ms = 1;
int64 end_time_ms = 2;
}
message Page {
int32 size = 1; // 0 = server default; server may cap
string token = 2; // opaque continuation (see below)
}
enum ListSort {
LIST_SORT_UNSPECIFIED = 0; // = admission ascending
LIST_SORT_ADMITTED_ASC = 1;
LIST_SORT_ADMITTED_DESC = 2;
}Small things folded in:
3. In-flight requests in the time windowOne edge case worth a look: the contract says "lifecycles overlap a time window," but the storage filter (parent PR) is 4. Pagination token — a note and an open questionEmbedding the query in the token looks safe: values go through bound Optional for later: if we ever want forged tokens to be impossible rather than just harmless, we could HMAC-sign the token with a server-side secret. The signature also guarantees integrity, so the token could shrink to position + a query hash instead of echoing the whole query. Cost is key management, so probably a follow-up unless the token needs to carry something sensitive. A length cap before decoding is a cheap safeguard regardless. Open one, no strong opinion —
Curious what you think — feels like a judgment call, and it interacts with the HMAC question. 5. Tiny things
Let me know your thoughts? |
| // The status is eventually consistent with the request store and reconciled from the append-only request log. | ||
| rpc Status(StatusRequest) returns (StatusResponse) {} | ||
|
|
||
| // List returns request summaries for one queue whose lifecycles overlap a time window. |
There was a problem hiding this comment.
I can't mentally parse the description :)
| func (c *ListController) List(ctx context.Context, req *pb.ListRequest) (*pb.ListResponse, error) { | ||
| start := time.Now() | ||
| defer func() { | ||
| c.metricsScope.Timer("list_latency").Record(time.Since(start)) |
There was a problem hiding this comment.
plz no timers.
use platform wrappers i.e. https://github.com/uber/submitqueue/blob/main/submitqueue/gateway/controller/land.go#L95
|
|
||
| if _, err := c.queueConfigs.Get(ctx, req.Queue); err != nil { | ||
| if errors.Is(err, queueconfig.ErrNotFound) { | ||
| return nil, errs.NewUserError(&UnrecognizedQueueError{Queue: req.Queue}) |
There was a problem hiding this comment.
req.Queue == ""
should also be a user error then? But it is not.
we need to design an error classification propagation for gateways/grpc, similar to orchestrator/queue
| limit = defaultListPageSize | ||
| } | ||
| if limit > maxListPageSize { | ||
| limit = maxListPageSize |
There was a problem hiding this comment.
I suggest to error instead
| return base64.RawURLEncoding.EncodeToString(data), nil | ||
| } | ||
|
|
||
| func decodeListPageToken(raw string) (*listPageToken, error) { |
|
|
||
| func decodeListPageToken(raw string) (*listPageToken, error) { | ||
| if raw == "" { | ||
| return nil, nil |
There was a problem hiding this comment.
why is this not an error?
| if !entity.IsKnownRequestStatus(status) { | ||
| return nil, fmt.Errorf("unknown status %q: %w", statusRaw, ErrInvalidRequest) | ||
| } | ||
| if _, ok := seen[status]; ok { |
There was a problem hiding this comment.
nit: can always assign to a set, then after the iteration completes, transform to a sorted list.
| return out | ||
| } | ||
|
|
||
| func equalStrings(a, b []string) bool { |
|
user inpuit validation could be extracted into a separate function |
788ef8d to
48dfb0b
Compare
d308ee7 to
27d33c8
Compare
48dfb0b to
c0e7f96
Compare
27d33c8 to
12b84dc
Compare
c0e7f96 to
c82b235
Compare
12b84dc to
418983a
Compare
c82b235 to
d565bc9
Compare
418983a to
4010630
Compare
Summary
Adds the public gateway List RPC and controller for queue-scoped request summaries with queue validation, status filtering, deterministic pagination, and admission-time sorting. Gateway write paths now persist log entries through the summary-aware helper so the read model stays updated.
Test Plan
✅
make fmt && make build && make test && make check-mocks && make e2e-testIssues
Stack